Skip to content

Fix culture-dependent number conversion and large JSON integer literals - #225

Open
Mike Krüger (mkrueger) wants to merge 2 commits into
mainfrom
dev/mkrueger/fix-script-number-handling
Open

Mike Krüger (mkrueger) wants to merge 2 commits into
mainfrom
dev/mkrueger/fix-script-number-handling

Conversation

@mkrueger

Copy link
Copy Markdown
Collaborator

Problem

Two bugs in how the scripting language handles numbers:

  1. Culture-dependent text-to-number conversion. ShellText and ShellIdentifier parsed numbers with the current culture. On a de-DE system, . is a group separator, so $t = "1.5"; echo $($t * 2.0) printed 30 and $($t - 0.5) printed 14.5, silently producing wrong values.
  2. Large integers in JSON literals were rejected. JSON object and array literals reused the Int32-only integer parsing, so mkitem {"id":"1","ts":1727600000000} failed with Invalid number format. Millisecond timestamps and other 64-bit identifiers are common in Cosmos DB documents.

Changes

Culture-invariant conversion (Use invariant numeric shell conversions)

  • Numeric parsing in ShellText, ShellIdentifier, ShellJson, ShellObject, and the remaining ExpressionParser integer paths uses NumberStyles with CultureInfo.InvariantCulture, matching how the lexer already parses decimal literals.

Large JSON integers (Accept large JSON integer literals)

  • In JSON object and array value positions, integer-form tokens outside Int32, including a leading unary -, become JSON number constants. The literal digits are preserved exactly, including values beyond 2^53. For example, {"n":9007199254740993} is stored without precision loss or a .0 suffix.
  • Plain integer literals outside JSON keep the documented 0..2147483647 limit.
  • Arithmetic on such properties follows the documented rule that larger JSON integers use double.

docs/programming.md documents both behaviors.

Tests

  • CultureInvariantConversionTests run under de-DE: text and identifier conversions, end-to-end expression evaluation, and rejection of "1,5".
  • ExpressionTests cover large positive and negative integers in objects, arrays, and nested objects, values just above Int32.MaxValue and beyond 2^53, arithmetic on such properties, and the unchanged error for plain 2147483648.

Parse shell text, identifiers, and related parser conversions with invariant numeric styles so decimal text is not interpreted through the current culture. Add de-DE regression coverage for text decimal arithmetic.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Preserve out-of-range integer-form JSON literal values as raw JSON numbers while keeping plain integer expressions limited to Int32.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation is targeted, documented, and covered by relevant regression tests.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes culture-dependent numeric conversion and preserves large integer literals in JSON construction.

Changes:

  • Uses invariant culture for numeric parsing.
  • Supports exact large JSON integer literals, including negatives.
  • Adds focused regression tests and programming documentation.
File Description
docs/​programming.md Documents invariant conversion and large JSON integers.
ShellText.cs Parses numeric text invariantly.
ShellObject.cs Uses invariant integer parsing.
ShellJson.cs Parses numeric JSON strings invariantly.
ShellIdentifier.cs Parses numeric identifiers invariantly.
ExpressionParser.cs Supports large JSON integer literals.
ExpressionTests.cs Tests large integer parsing and arithmetic.
CultureInvariantConversionTests.cs Tests conversion under German culture.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-code-quality

Copy link
Copy Markdown

Code Coverage Overview

Languages: C#

C# / code-coverage/dotnet

The overall line coverage in commit a93f20c in the dev/mkrueger/fix-scr... branch remains at 64%, unchanged from commit 345ddb8 in the main branch.

Show a line coverage summary of the most impacted files.
File main 345ddb8 dev/mkrueger/fix-scr... a93f20c +/-
D:\a\CosmosDBSh...essionParser.cs 84% 84% 0%
D:\a\CosmosDBSh...ct\ShellText.cs 93% 96% +3%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants